-
-
Notifications
You must be signed in to change notification settings - Fork 8.1k
feat: Implement site name aliases #2581
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
feat: Implement site name aliases #2581
Conversation
Automatic validation of changes
Failures were detected on at least one updated target. Commits containing accuracy failures will often not be merged (unless a rationale is provided, such as false negatives due to regional differences). |
Automatic validation of changes
Failures were detected on at least one updated target. Commits containing accuracy failures will often not be merged (unless a rationale is provided, such as false negatives due to regional differences). |
Automatic validation of changes
Failures were detected on at least one updated target. Commits containing accuracy failures will often not be merged (unless a rationale is provided, such as false negatives due to regional differences). |
Hi @shreyasNaik0101! I tested your PR and found the alias matching wasn't working ( What needs to be changed:1. sherlock_project/sherlock.py - Update the site selection logic to check aliases Lines (828 - 864)
2. sherlock_project/resources/data.json - Remove broken Twitter Lines (2186 - 2197)
You can see my working implementation here for reference: Test results after fix:$ sherlock narendramodi --site Twitter
[+] Twitter: https://x.com/narendramodi
$ sherlock narendramodi --site X
[+] Twitter: https://x.com/narendramodi Both work perfectly! ✅ If you'd like, you can apply these changes to your PR. And if you use my code, feel free to add me as a co-author using:
Let me know if you need any help! Resolves issue #2402 |
Automatic validation of changes
Failures were detected on at least one updated target. Commits containing accuracy failures will often not be merged (unless a rationale is provided, such as false negatives due to regional differences). |
Co-authored-by: obiwan04kanobi <[email protected]>
9273137
to
91ba5a4
Compare
Automatic validation of changes
Failures were detected on at least one updated target. Commits containing accuracy failures will often not be merged (unless a rationale is provided, such as false negatives due to regional differences). |
Hello, I've fixed the JSONDecodeError and the local schema validation is now passing. The only remaining failure is in test_validate_manifest_against_remote_schema, which is failing because the remote schema doesn't know about the new aliases property. Could you please guide me on how to resolve this last validation error? |
Hi @shreyasNaik0101, I analyzed all 12 test failure logs across different platforms and Python versions. Every single test shows the same pattern: 1 failed, 24 passed. The only failure is
This is the expected failure The test compares our The PR is technically sound. Once maintainers merge this, the remote schema will be updated and this test will pass. This is standard for schema-changing PRs. @ppfeister - Could you take a look when you have a moment? The implementation is working correctly (both |
Schema changes are expected to cause some test failures, no worries. This will still be validated independently (once there's time, ofc) |
Dumb issue, but how much work would it be to retain the two-space indent on the schema? Noticing the diff is the entire file, making it a pain to review with line diff (i.e. what GH uses) vs word diff |
@ppfeister Thanks for the quick response! Regarding the indent issue - that's a valid concern. The diff is showing the entire file because the indentation was changed from 2 to 4 spaces. @shreyasNaik0101 - Would you be able to revert to 2-space indentation for the schema file? That way the diff will only show the actual changes (adding The actual functional changes are minimal:
Everything else is just indentation changes. |
I haven't reviewed the full diff but worth noting, username unclaimed was deprecated quite a while ago and should not be re-added Will give a more proper review when actually at a desk and free |
# Create original dictionary from SitesInformation() object. | ||
# Eventually, the rest of the code will be updated to use the new object | ||
# directly, but this will glue the two pieces together. | ||
site_data_all = {site.name: site.information for site in sites} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On lines 786-789:
@shreyasNaik0101 Small cleanup needed - this line appears to be a duplicate of line 786. Can you remove the duplicate?
site_data_all = {site.name: site.information for site in sites} # Create original dictionary from SitesInformation() object. ← Duplicate # Eventually, the rest of the code will be updated to use the new object ← Duplicate # directly, but this will glue the two pieces together. ← Duplicate site_data_all = {site.name: site.information for site in sites} # ← DuplicateOne of these should be removed. Thanks!
Automatic validation of changes
Failures were detected on at least one updated target. Commits containing accuracy failures will often not be merged (unless a rationale is provided, such as false negatives due to regional differences). |
Automatic validation of changes
Failures were detected on at least one updated target. Commits containing accuracy failures will often not be merged (unless a rationale is provided, such as false negatives due to regional differences). |
I’ve implemented the suggested changes. Please review and let me know if any further modifications are required. |
This PR implements site name aliases.
aliases
array.--site
filter.Closes #2373.